Skip to content

Conversation

@nsajko
Copy link
Member

@nsajko nsajko commented Apr 9, 2025

Iterators with HasShape{N} are certainly the most well-behaved class of iterators in Julia. Currently several separate type trees of such iterators come with Julia. It seems like joining these trees under a single abstract type would be nice.

Why this change is useful:

  • Make expressing hardcoded Union types for zero-dimensional collections unnecessary: instead of Union{Number,Ref,AbstractArray{<:Any,0}} now it will be possible to write simply, and more generically, ShapefulIterator{0}. At least in some cases.
  • Reduce the number of ndims and IteratorSize methods:
    • The number of ndims methods that come with Julia is decreased by six
    • The number of IteratorSize methods that come with Julia is decreased by three

Why this change doesn't prevent a better future supertype choice:

  • Naively it might make sense to have Number, AbstractChar and Ref subtype a parameterless abstract type like ZeroDimensionalIterator. However:
    • This seems less useful than ShapefulIterator{N}
    • Adding ShapefulIterator doesn't preclude adding ZeroDimensionalIterator <: ShapefulIterator{0} later.
  • One might ask: why not make AbstractArray{T} subtype a different abstract type, with an element type type parameter, something like AbstractArray{T,N} <: TypedIterator{T}. In this case, however, TypedIterator would be useless for the zero dimensional iterators mentioned above, as they are their own element types, something not expressible in abstract type subtyping.

@nsajko nsajko added arrays [a, r, r, a, y, s] design Design of APIs or of the language itself broadcast Applying a function over a collection iteration Involves iteration or the iteration protocol feature Indicates new feature / enhancement requests labels Apr 9, 2025
ndims(c::AbstractChar) = 0
ndims(::Type{<:AbstractChar}) = 0
length(c::AbstractChar) = 1
IteratorSize(::Type{Char}) = HasShape{0}()
Copy link
Member Author

@nsajko nsajko Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: here IteratorSize was defined for Char. With this PR it's defined for all AbstractChar subtypes (making this kinda a minor change). However that's OK, because this method should have been IteratorSize(::Type{AbstractChar}) = HasShape{0}() in the first place. Refer to:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ndims being defined for AbstractChar implies IteratorSize should be treated the same, I'd say.

@nsajko nsajko force-pushed the new_abstract_type_ShapefulIterator branch 2 times, most recently from 5754426 to c4e8f44 Compare April 9, 2025 02:29
@nsajko
Copy link
Member Author

nsajko commented Apr 9, 2025

Notes:

  • Initially I tried making the new type public from Base but not export-ed. However this is not acceptable with how types (in general) are currently printed because it gets shown as Core.ShapefulIterator instead of as Base.ShapefulIterator.
  • A LinearAlgebra.jl doc test is failing, this will require coordination with the stdlib if it gets decided this PR should be merged.

@nsajko
Copy link
Member Author

nsajko commented Apr 9, 2025

An interesting consequence of this PR is that [3, [7]] is not Vector{Any} any more:

julia> typeof([3, [7]])
Vector{ShapefulIterator} (alias for Array{ShapefulIterator, 1})

@nsajko nsajko added the needs pkgeval Tests for all registered packages should be run with this change label Apr 9, 2025
Iterators with `HasShape{N}` are certainly the most well-behaved class
of iterators in Julia. Currently several separate type trees of such
iterators come with Julia. It seems like joining these trees under a
single abstract type would be nice.

Why this change is useful:
* Make expressing hardcoded `Union` types for zero-dimensional
  collections unnecessary: instead of
  `Union{Number,Ref,AbstractArray{<:Any,0}}` now it will be possible to
  write simply, and more generically, `ShapefulIterator{0}`. At
  least in some cases.
* Reduce the number of `ndims` and `IteratorSize` methods:
    * The number of `ndims` methods that come with Julia is
      decreased by six
    * The number of `IteratorSize` methods that come with Julia is
      decreased by three

Why this change doesn't prevent a better future supertype choice:
* Naively it might make sense to have `Number`, `AbstractChar` and
  `Ref` subtype a parameterless abstract type like
  `ZeroDimensionalIterator`. However:
    * This seems less useful than `ShapefulIterator{N}`
    * Adding `ShapefulIterator` doesn't preclude adding
      `ZeroDimensionalIterator <: ShapefulIterator{0}` later.
* One might ask: why not make `AbstractArray{T}` subtype a different
  abstract type, with an element type type parameter, something like
  `AbstractArray{T,N} <: TypedIterator{T}`. In this case, however,
  `TypedIterator` would be useless for the zero dimensional iterators
  mentioned above, as they are their own element types, something not
  expressible in abstract type subtyping.
@nsajko nsajko force-pushed the new_abstract_type_ShapefulIterator branch from c4e8f44 to 70c2516 Compare April 9, 2025 04:26
@nsajko nsajko added the speculative Whether the change will be implemented is speculative label Apr 14, 2025
@nsajko
Copy link
Member Author

nsajko commented Apr 15, 2025

Giving up on this, at least for now, as I have too many open PRs to handle.

@nsajko nsajko closed this Apr 15, 2025
@nsajko nsajko deleted the new_abstract_type_ShapefulIterator branch April 15, 2025 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrays [a, r, r, a, y, s] broadcast Applying a function over a collection design Design of APIs or of the language itself feature Indicates new feature / enhancement requests iteration Involves iteration or the iteration protocol needs pkgeval Tests for all registered packages should be run with this change speculative Whether the change will be implemented is speculative

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant